Skip to content

add deprecation notice for mapbox traces #4783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 7, 2024

Closes #4730.

@archmoj archmoj requested a review from LiamConnors October 7, 2024 18:28
@gvwilson gvwilson requested a review from ndrezn October 8, 2024 11:48
@gvwilson gvwilson added feature something new P1 needed for current cycle labels Oct 8, 2024
@LiamConnors
Copy link
Member

LiamConnors commented Oct 8, 2024

These show up when you are using the new traces

image

Also, it looks like they are removed when we run the commands to update Plotly.js. Those classes are probably autogenerated

@gvwilson
Copy link
Contributor

gvwilson commented Oct 8, 2024

cc @archmoj

print(
"*choroplethmapbox* trace is deprecated!",
"Please consider switching to the *choroplethmap* trace type and `map` subplots.",
"Learn more at: https://plotly.com/javascript/maplibre-migration/",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All links should point to the Python documentation: https://plotly.com/python/mapbox-to-maplibre/

@@ -3,6 +3,11 @@


class Choroplethmapbox(_BaseTraceType):
print(
Copy link
Member

@ndrezn ndrezn Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a proper warning rather than print statements

@archmoj archmoj assigned emilykl and unassigned archmoj Oct 17, 2024
@gvwilson gvwilson assigned archmoj and unassigned emilykl Oct 24, 2024
@archmoj archmoj requested a review from ndrezn November 6, 2024 19:16
@LiamConnors
Copy link
Member

could we use a warning as mentioned by @ndrezn above #4783 (comment)

@archmoj
Copy link
Contributor Author

archmoj commented Nov 6, 2024

could we use a warning as mentioned by @ndrezn above #4783 (comment)

I am not sure is that's a good idea.
My concern is that it could be too annoying for users and it may be considered a notable undesirable change.

@emilykl
Copy link
Contributor

emilykl commented Nov 6, 2024

could we use a warning as mentioned by @ndrezn above #4783 (comment)

I am not sure is that's a good idea. My concern is that it could be too annoying for users and it may be considered a notable undesirable change.

@archmoj Being annoying is the whole point, so that users will change their code before the traces are removed entirely.

@archmoj
Copy link
Contributor Author

archmoj commented Nov 7, 2024

@LiamConnors Could you please test this to see if this produce the desirable warnings?
Thank you!

@archmoj archmoj requested a review from LiamConnors November 7, 2024 16:04
@LiamConnors
Copy link
Member

@archmoj, I see the deprecation notice on traces and px functions that don't use mapbox

import plotly.express as px
df = px.data.gapminder().query("year == 2007")
fig = px.line_geo(df, locations="iso_alpha",
                  color="continent", # "continent" is one of the columns of gapminder
                  projection="orthographic")
fig.show()
image image

But not on ones that do:

image

@emilykl
Copy link
Contributor

emilykl commented Nov 7, 2024

@archmoj Maybe you're already on it but I think the warning needs to go inside the __init__ function rather than at the top of the class to fix the issue Liam is seeing.

@archmoj
Copy link
Contributor Author

archmoj commented Nov 7, 2024

@archmoj Maybe you're already on it but I think the warning needs to go inside the __init__ function rather than at the top of the class to fix the issue Liam is seeing.

@LiamConnors Thanks for catching that. Let's try @emilykl's suggestion.

@LiamConnors
Copy link
Member

@archmoj, I still see the issue

image

@archmoj
Copy link
Contributor Author

archmoj commented Nov 13, 2024

@LiamConnors Debugging the bug you noticed wasn't easy. I figured the error is coming from the plotly.py templates which include defaults for "scattermapbox" traces as well as `"mapbox" subplots.
On the other hand resolving the merge conflicts of this PR is not easy.
So I close this PR; then open a new PR.
Thank you!

@archmoj archmoj closed this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add deprecation warning for Mapbox traces
5 participants